Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove contiguous from reshape parsing #2190

Merged
merged 8 commits into from
Oct 10, 2023
Merged

Remove contiguous from reshape parsing #2190

merged 8 commits into from
Oct 10, 2023

Conversation

TedThemistokleous
Copy link
Collaborator

Related to #2099

Cleaning up parsing reshape which adds additional contiguous

@TedThemistokleous TedThemistokleous self-assigned this Sep 14, 2023
@TedThemistokleous TedThemistokleous added simple small or simple changes roadmap Tasks to finish for a release labels Sep 14, 2023
@TedThemistokleous TedThemistokleous linked an issue Sep 14, 2023 that may be closed by this pull request
Cleanup contiguous from Reshape parsing #2187
Closed
@migraphx-bot
Copy link
Collaborator

migraphx-bot commented Sep 15, 2023 •
edited
Loading

Test Batch Rate new
2f6798		 Rate old
f5411e		 Diff Compare
torchvision-resnet50 64 2,325.01 2,323.57 0.06% ✅
torchvision-resnet50_fp16 64 5,355.25 5,356.19 -0.02% ✅
torchvision-densenet121 32 1,847.97 1,832.62 0.84% ✅
torchvision-densenet121_fp16 32 3,415.30 3,419.52 -0.12% ✅
torchvision-inceptionv3 32 1,295.90 1,293.47 0.19% ✅
torchvision-inceptionv3_fp16 32 2,534.85 2,531.69 0.13% ✅
cadene-inceptionv4 16 619.77 620.19 -0.07% ✅
cadene-resnext64x4 16 588.49 588.91 -0.07% ✅
slim-mobilenet 64 7,200.70 7,215.28 -0.20% ✅
slim-nasnetalarge 64 236.51 236.38 0.05% ✅
slim-resnet50v2 64 2,557.35 2,556.82 0.02% ✅
bert-mrpc-onnx 8 824.08 826.08 -0.24% ✅
bert-mrpc-tf 1 389.19 388.61 0.15% ✅
pytorch-examples-wlang-gru 1 296.41 297.89 -0.50% ✅
pytorch-examples-wlang-lstm 1 314.71 307.72 2.27% ✅
torchvision-resnet50_1 1 550.51 543.08 1.37% ✅
torchvision-inceptionv3_1 1 305.24 305.76 -0.17% ✅
cadene-dpn92_1 1 355.23 355.84 -0.17% ✅
cadene-resnext101_1 1 219.65 218.19 0.67% ✅
slim-vgg16_1 1 223.95 223.82 0.06% ✅
slim-mobilenet_1 1 1,485.18 1,523.59 -2.52% ✅
slim-inceptionv4_1 1 217.61 215.72 0.88% ✅
onnx-taau-downsample 1 304.33 306.61 -0.75% ✅
dlrm-criteoterabyte 1 21.68 21.69 -0.05% ✅
dlrm-criteoterabyte_fp16 1 40.73 40.72 0.02% ✅
agentmodel 1 5,843.78 5,860.43 -0.28% ✅
unet_fp16 2 55.21 55.16 0.09% ✅
resnet50v1_fp16 1 754.19 789.31 -4.45% 🔴
bert_base_cased_fp16 64 970.92 970.39 0.05% ✅
bert_large_uncased_fp16 32 304.96 304.93 0.01% ✅
bert_large_fp16 1 166.68 166.80 -0.07% ✅
distilgpt2_fp16 16 1,278.48 1,351.09 -5.37% 🔴

This build is not recommended to merge 🔴

@migraphx-bot
Copy link
Collaborator

migraphx-bot commented Sep 15, 2023 •
edited
Loading


    :white_check_mark:bert-mrpc-onnx: PASSED: MIGraphX meets tolerance

    :white_check_mark:bert-mrpc-tf: PASSED: MIGraphX meets tolerance

    :white_check_mark:pytorch-examples-wlang-gru: PASSED: MIGraphX meets tolerance

    :white_check_mark:pytorch-examples-wlang-lstm: PASSED: MIGraphX meets tolerance

    :white_check_mark:torchvision-resnet50_1: PASSED: MIGraphX meets tolerance

    :white_check_mark:torchvision-inceptionv3_1: PASSED: MIGraphX meets tolerance

    :white_check_mark:cadene-dpn92_1: PASSED: MIGraphX meets tolerance

    :white_check_mark:cadene-resnext101_1: PASSED: MIGraphX meets tolerance

    :white_check_mark:slim-vgg16_1: PASSED: MIGraphX meets tolerance

    :white_check_mark:slim-mobilenet_1: PASSED: MIGraphX meets tolerance

    :white_check_mark:slim-inceptionv4_1: PASSED: MIGraphX meets tolerance

    :white_check_mark:dlrm-criteoterabyte: PASSED: MIGraphX meets tolerance

    :white_check_mark:agentmodel: PASSED: MIGraphX meets tolerance

    :white_check_mark:unet: PASSED: MIGraphX meets tolerance

    :white_check_mark:resnet50v1: PASSED: MIGraphX meets tolerance

🔴bert_base_cased_fp16: FAILED: MIGraphX is not within tolerance - check verbose output


🔴bert_large_uncased_fp16: FAILED: MIGraphX is not within tolerance - check verbose output


    :white_check_mark:bert_large: PASSED: MIGraphX meets tolerance

🔴distilgpt2_fp16: FAILED: MIGraphX is not within tolerance - check verbose output

@TedThemistokleous TedThemistokleous marked this pull request as ready for review September 20, 2023 22:48
CharlieL7
CharlieL7 requested changes Sep 27, 2023
View reviewed changes
Copy link
Collaborator

@CharlieL7 CharlieL7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take a look at parse_spacetodepth.cpp, parse_depthtospace.cpp and simplify_reshapes. It looks like there are other instances where contiguous are added to reshape.

@TedThemistokleous
Copy link
Collaborator Author

Take a look at parse_spacetodepth.cpp, parse_depthtospace.cpp and simplify_reshapes. It looks like there are other instances where contiguous are added to reshape.

Will do. I'll get these fixed. Thanks!

@TedThemistokleous
Copy link
Collaborator Author

Rebased off develop as I wanted to pull in merged changes from #2099

@TedThemistokleous TedThemistokleous added the Cleanup Cleans up code from stale bits/warnings/previous changes for a previous feature PR label Sep 28, 2023
@codecov
Copy link

codecov bot commented Sep 28, 2023 •
edited
Loading

Codecov Report

Merging #2190 (b77a818) into develop (a3cf995) will increase coverage by 0.04%.
The diff coverage is 100.00%.

❗ Current head b77a818 differs from pull request most recent head 5b82610. Consider uploading reports for the commit 5b82610 to get more accurate results

@@             Coverage Diff             @@
##           develop    #2190      +/-   ##
===========================================
+ Coverage    91.45%   91.50%   +0.04%     
===========================================
  Files          433      431       -2     
  Lines        16177    16136      -41     
===========================================
- Hits         14795    14765      -30     
+ Misses        1382     1371      -11
Files Coverage Δ
src/onnx/parse_depthtospace.cpp 90.90% <100.00%> (-0.27%) ⬇️
src/onnx/parse_reshape.cpp 100.00% <100.00%> (ø)
src/onnx/parse_spacetodepth.cpp 100.00% <100.00%> (ø)
src/tf/parse_reshape.cpp 85.71% <100.00%> (-1.79%) ⬇️

... and 5 files with indirect coverage changes

CharlieL7
CharlieL7 approved these changes Oct 4, 2023
View reviewed changes
Copy link
Collaborator

@CharlieL7 CharlieL7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, wasn't sure about the simplify_algebra reshape stuff. Or compiler passes adding contiguous -> reshape in general.

@TedThemistokleous
Copy link
Collaborator Author

guessing server reset again - Seeing this on onnx server run

Unable to create live FilePath for rocm-framework-33; rocm-framework-33 was marked offline: Disconnected by z1_miciadmin : cliinfofailure

@CharlieL7 CharlieL7 mentioned this pull request Oct 6, 2023
Merged
umangyadav
umangyadav approved these changes Oct 10, 2023
View reviewed changes
Copy link
Member

@umangyadav umangyadav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please open an issue about removing contiguous from simplify_algebra/reshapes and other passes.

@causten causten merged commit e5cd8b6 into develop Oct 10, 2023
14 of 15 checks passed
@causten causten deleted the reshape_remove_contiguous branch October 10, 2023 15:30
This was referenced Oct 10, 2023
Closed
Merged
@umangyadav umangyadav mentioned this pull request Oct 17, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@umangyadav umangyadav umangyadav approved these changes

@CharlieL7 CharlieL7 CharlieL7 approved these changes

@pfultz2 pfultz2 Awaiting requested review from pfultz2

Assignees

@TedThemistokleous TedThemistokleous

Labels
Cleanup Cleans up code from stale bits/warnings/previous changes for a previous feature PR roadmap Tasks to finish for a release simple small or simple changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cleanup contiguous from Reshape parsing
5 participants
@TedThemistokleous @migraphx-bot @CharlieL7 @umangyadav @causten